-
Notifications
You must be signed in to change notification settings - Fork 414
Update Tooltip visual design in interface guidelines + accessibility guidelines
#565
Conversation
| alt="tooltips rendered in all possible positions surrounding a control" | ||
| /> | ||
|
|
||
| ## Alternatives to tooltips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this section so much
khiga8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass! Thank you for working on this :)
I hope to take a closer look tomorrow!
| alt="tooltips rendered in all possible positions surrounding a control" | ||
| /> | ||
|
|
||
| ## Alternatives to tooltips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a couple of lint rules and doc pages where we cross-reference the existing Tooltip alternatives docs. Let's make sure to update those to avoid a dead link! (I can verify tomorrow which ones those are).
Or could it be sufficient to just cross-reference that page from this section rather than moving it entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow up with the links outside this repo once this PR merges! ❤️ thank you
| If you are unsure which alternative is more suited to your scenario and need help, consult a designer or the GitHub Accessibility team (if you are GitHub staff) for advice. | ||
|
|
||
| Most UI cases don't call for tooltips. Learn some [alternative methods to use in place of tooltips](/../guides/accessibility/tooltip-alternatives). | ||
| ## Additional resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We introduced a new PVC lint rule recently the deprecated Primer CSS tooltipped which we have a ton of debt around that we want to tackle. It looks like the lint rule doc is now on Primer CSS Tooltipped.
Could it make sense to link to it here? :)
|
|
||
| ## Options | ||
|
|
||
| ### Label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be useful to talk about the underlying markup of the differences between a label and description (e.g. aria-labelledby vs aria-describedby)? One will serve as the accessible name of a control, while the other will supplement it.
We have some more documentation in Primer Rails tooltip (This was originally under an accessibility section in Primer Rails but I think it changed with the recent docs site update!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you! I had a hard time trying to explain this one so let me see if I can rework it a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people 🤔
(cc: @broccolinisoup who worked on React tooltip :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you! I had a hard time trying to explain this one so let me see if I can rework it a bit.
Katie let me know if you want to pair on this or any help with it!
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people
Yes! We are going to ship the draft version soon hopefully 🙏🏻 and also the visual changes (i.e. removing the caret) also will be shipped with the draft version so holding off merging this PR until the draft is out would be wise.
lukasoppermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
|
Hey @langermank could you add a Figma section as well? |
| alt="tooltips rendered in all possible positions surrounding a control" | ||
| /> | ||
|
|
||
| ## Alternatives to tooltips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| ## Options | ||
|
|
||
| ### Label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people 🤔
(cc: @broccolinisoup who worked on React tooltip :) )
content/components/tooltip.mdx
Outdated
| For interactive controls that require additional context, a tooltip can be used to provide it. The `label` is also | ||
| required, and the description will be announced to a screen reader in addition to the label. Keep the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will want to reword this a bit since this seems to suggest one should set both the label and description prop of the tooltip API which I don't think is the case. Maybe we want to say that when a tooltip supplements a control as a description, we should make sure the control has an accessible name?
<button aria-describedby="tooltip-1">Name of button</button>
<tooltip id="tooltip-1">Some supplementary description</tooltip>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make sure the control has an accessible name?
I agree with this. Since accessible name could be a visible button text or an aria-label, we can maybe divide the "Description" section into two subsections?
Description for buttons that has a visible name
<button aria-describedby="tooltip-1">Name of button</button>
<tooltip id="tooltip-1">Some supplementary description</tooltip>Description for buttons that doesn't have a visible name
<iconbutton aria-label="icon-button-label" />
<tooltip id="tooltip-1">Some supplementary description</tooltip>|
|
||
| ## Options | ||
|
|
||
| ### Label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we mention how a label type tooltip should be reserved for icon buttons? The underlying semantic of a label tooltip is aria-labelledby would override any label the button already has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is clear already by saying "For interactive controls with no visible text label" but I will add "such as icon buttons" 😄
broccolinisoup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So cool 🔥 Left some comments, let me know what you think 🙌🏻
|
|
||
| ## Options | ||
|
|
||
| ### Label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you! I had a hard time trying to explain this one so let me see if I can rework it a bit.
Katie let me know if you want to pair on this or any help with it!
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people
Yes! We are going to ship the draft version soon hopefully 🙏🏻 and also the visual changes (i.e. removing the caret) also will be shipped with the draft version so holding off merging this PR until the draft is out would be wise.
content/components/tooltip.mdx
Outdated
| For interactive controls that require additional context, a tooltip can be used to provide it. The `label` is also | ||
| required, and the description will be announced to a screen reader in addition to the label. Keep the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make sure the control has an accessible name?
I agree with this. Since accessible name could be a visible button text or an aria-label, we can maybe divide the "Description" section into two subsections?
Description for buttons that has a visible name
<button aria-describedby="tooltip-1">Name of button</button>
<tooltip id="tooltip-1">Some supplementary description</tooltip>Description for buttons that doesn't have a visible name
<iconbutton aria-label="icon-button-label" />
<tooltip id="tooltip-1">Some supplementary description</tooltip>Co-authored-by: Kate Higa <[email protected]>
Co-authored-by: Kate Higa <[email protected]>
Co-authored-by: Kate Higa <[email protected]>
Co-authored-by: Kate Higa <[email protected]>
Goals
Accessibility page → https://primer-14e523666e-26441320.drafts.github.io/guides/accessibility/tooltip-alternatives
Interface guidelines → https://primer-14e523666e-26441320.drafts.github.io/components/tooltip
TODO